Calling require with null char as first char of the path component causes crash #13788
Conversation
nodejs-github-bot
added
the
module
|
Why ignore the rest silently? Throw an error. |
|
See also #8277 and, uh, #8277 (comment). General observation: naively splitting the string like that probably introduces an unacceptable performance regression. Would need to be checked. |
Null char as the first char as the path component of first argument of require causes a node crash. Ignoring null and all chars after that in require path. Fixes: nodejs#13787
Null char as the first char as the path component of first argument of require causes a node crash. Ignoring null and all chars after that in require path. Fixes: nodejs#13787
|
@charmander: Yes, we can do that. Throw error on any string with null. Looks like that the behavior as per #8277 @bnoordhuis : if we throw on mere presence of null, split can be removed. But hard to remove single pass. |
|
@zimbabao I don't think it matters whether there are multiple passes since having a null character in the string is not a common case. |
|
@mscdex : But we have to run that on all the paths, latency due to reading from disk is high as compared to this check. |
|
@zimbabao What I meant was having a simpler |
Null char as the first char as the path component of first argument of require causes a node crash. Ignoring null and all chars after that in require path. Fixes: nodejs#13787
lib/module.js
Outdated
| @@ -510,6 +510,10 @@ Module.prototype.load = function(filename) { | |||
| Module.prototype.require = function(path) { | |||
| assert(path, 'missing path'); | |||
| assert(typeof path === 'string', 'path must be a string'); | |||
| // Ignore part of the path after null character if it exists | |||
| if (path.indexOf('\u0000') != -1) { | |||
There was a problem hiding this comment.
Can we just do:
if (path.includes('\u0000') {
There was a problem hiding this comment.
I just ran a simple benchmark to compare the two on master and it seems indexOf() is still quite a bit faster overall.
There was a problem hiding this comment.
However, we should use double equals here: !== -1
|
It would be good to see some |
Null char as the first char as the path component of first argument of require causes a node crash. Ignoring null and all chars after that in require path. Fixes: nodejs#13787
|
@charmander : #8277 throws error if path contains null bytes, and #8292 is dependent PR. I'm not sure why its not being pursued. |
|
This got superseded by ffed7b6 |
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)